Skip to content

feat(ui): Wave 0 foundations for AntD → ui-core-components migration - #30560

Merged
chirag-madlani merged 20 commits into
mainfrom
antd-migration/wave-0-foundations
Jul 30, 2026
Merged

feat(ui): Wave 0 foundations for AntD → ui-core-components migration#30560
chirag-madlani merged 20 commits into
mainfrom
antd-migration/wave-0-foundations

Conversation

@chirag-madlani

@chirag-madlani chirag-madlani commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

Foundations for the AntD → @openmetadata/ui-core-components migration program (Wave 0). No product code changes — tooling, docs, test harness, and CI only.

  • docs/antd-migration/ — sweep playbook README + mapping-guide template (the review contract for every future component sweep)
  • tooling/antd-codemods/ — jscodeshift scaffold with a generic move-named-imports transform (6 inline tests: partial move, full-move removal, alias preservation, merge into existing import, no-op, license-header retention)
  • tooling/antd-migration/ledger.mjs — sweep-ledger generator counting antd imports per component per repo plus .ant-* selector occurrences (5 node:test cases; skips vendored/generated dirs; QueryBuilder files excluded as owned by feat(query-builder): migrate QueryBuilderWidgetV1 from antd to core-components #29849)
  • Visual-regression harness — new visual-regression Playwright project (separate from e2e; chromium project ignores it), determinism helper (fixed clock before navigation, animation freeze, shared screenshot opts), and 16 committed baselines: 13 static pages (incl. collapsed-sidebar variant), table entity details (API-provisioned fixture, name masks), RJSF connector config form, delete-confirmation modal
  • .github/workflows/playwright-visual.yml — PR gate for openmetadata-ui/** running the visual suite inside mcr.microsoft.com/playwright:v1.57.0-jammy (same image the baselines were generated in, for font-rendering parity), with disk-space reclaim and failure-artifact upload

Verification

  • Baselines were generated in the Playwright docker image (Linux rendering) against a locally seeded server, and the full suite passed two consecutive verification runs (18/18 each) with zero diffs.
  • tooling/antd-codemods: yarn test 6/6 · tooling/antd-migration: node --test 5/5 · eslint/prettier clean on all new spec files · actionlint clean on the workflow.
  • knip seed list (docs/antd-migration/dead-code-seed.json, 8 files / 451 issues): 3 entries grep-spot-checked, 0 false positives (e.g. DataAssetsHeaderExtraInfo has zero importers). Seed is advisory — deletions must be re-proven in the deleting PR.

Notes for reviewers

  • The visual workflow's first live run is the real end-to-end test of the docker/host-networking path in CI; if runner font rendering differs from the pinned image expectations, a one-time --update-snapshots pass regenerates baselines.
  • A sibling PR in the Collate repo (cross-repo ledger + collate-ui visual project) references docs added here, so this PR should merge first.

🤖 Generated with Claude Code

Fixes #30564 (Wave 0 of epic #30570)

Greptile Summary

This PR establishes tooling and CI foundations for the AntD migration.

  • Adds migration documentation, a dead-code seed, and an AntD usage ledger.
  • Adds a named-import codemod scaffold with tests.
  • Adds a Playwright visual-regression project, snapshots, deterministic screenshot helpers, and a dedicated workflow.
  • Delegates visual-regression specs away from the main Playwright workflow and tests that selection behavior.

Confidence Score: 3/5

The PR is not yet safe to merge because the codemod can convert selected inline type imports into value imports.

The transform partitions work using only declaration-level importKind, so mixed syntax such as import { Button, type TabsProps } sends TabsProps through the value pass and can generate an invalid runtime import; the existing tests cover only standalone import type declarations.

Files Needing Attention: tooling/antd-codemods/transforms/move-named-imports.js, tooling/antd-codemods/tests/move-named-imports.test.js

Important Files Changed

Filename Overview
tooling/antd-codemods/transforms/move-named-imports.js Adds the named-import migration transform, but mixed inline type specifiers remain routed through declaration-level value handling.
tooling/antd-codemods/tests/move-named-imports.test.js Covers standalone type-only declarations and destination-kind separation but omits mixed inline type imports.
tooling/antd-migration/ledger.mjs Adds the repository migration-ledger generator for AntD imports and selector usage.
openmetadata-ui/src/main/resources/ui/playwright.config.ts Adds a dedicated visual-regression Playwright project while excluding those specs from Chromium.
.github/workflows/playwright-visual.yml Adds the pinned-container visual-regression workflow and now pins the disk-space action to an immutable commit.
.github/playwright/impact-map.json Delegates visual-regression specs from the main Playwright test planner.

Reviews (7): Last reviewed commit: "Merge origin/main to pick up tw-deprecat..." | Re-trigger Greptile

chirag-madlani and others added 9 commits July 27, 2026 19:01
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sform

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 28, 2026 09:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically.

Maintainers can bypass this check by adding the skip-pr-checks label.

@github-actions github-actions Bot added safe to test Add this label to run secure Github workflows on PRs UI UI specific issues labels Jul 28, 2026
Comment thread tooling/antd-migration/ledger.mjs
Comment thread tooling/antd-codemods/transforms/move-named-imports.js Outdated
Comment thread tooling/antd-codemods/transforms/move-named-imports.js
Comment thread .github/workflows/playwright-visual.yml
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

✅ Playwright Results — workflow succeeded

Validated commit 24241fe277747048dbe319a49200a5661836747d in Playwright run 30431290018, attempt 1.

✅ 537 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 5 skipped · 🧰 0 lifecycle flaky

Performance

Blocking targets: ✅ met · Optimization targets: 🟡 in progress

Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting.

🕒 Full workflow signal wall (to summary) 48m 54s

⏱️ Max setup 2m 57s · max shard execution 14m 38s · max shard-job elapsed before upload 17m 49s · reporting 4s

🌐 203.09 requests/attempt · 2.86 app boots/UI scenario · 8.54% common-shard skew

Optimization targets still in progress:

  • Browser traffic was 203.09 requests per attempt (convergence target: fewer than 200).
  • Application boot ratio was 2.86 per UI scenario (1596 boots / 558 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard chromium-01 102 0 0 3 0 0
✅ Shard chromium-02 88 0 0 0 0 0
✅ Shard chromium-03 100 0 0 0 0 0
✅ Shard chromium-04 109 0 0 0 0 0
✅ Shard data-asset-rules-01 61 0 0 0 0 0
✅ Shard domain-isolation-01 14 0 0 0 0 0
✅ Shard global-state-01 23 0 0 0 0 0
✅ Shard ingestion-01 1 0 0 0 0 0
✅ Shard reindex-01 2 0 0 0 0 0
✅ Shard search-01 10 0 0 0 0 0
✅ Shard search-rbac-01 27 0 0 2 0 0

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
65.79% (76987/117008) 49.64% (46261/93179) 50.89% (13932/27375)

chirag-madlani and others added 4 commits July 28, 2026 15:11
playwright/e2e/VisualRegression/*.spec.ts is testIgnore'd by the
chromium project (it runs under its own playwright-visual.yml
workflow), but select_playwright_tests.py still emitted a selector
for changed files under that directory. build_playwright_shards.py
then rejected the plan because the selector matched no runnable
chromium test, crashing "plan-playwright" for any PR touching the
visual suite.

Add playwright/e2e/VisualRegression/** to impact-map.json's
delegatedSpecs, the existing mechanism used for other
dedicated-workflow suites (Auth, nightly, Http2, RDF specs), so
changed visual-regression specs are routed to
delegatedChangedSpecs instead of selectors.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reformat entityDetails.spec.ts to satisfy Prettier's line-length rule
(the UI Checkstyle job runs organize-imports-cli + eslint --fix +
prettier --write on changed playwright files and fails the PR if that
leaves a diff). Ran the same organize-imports/eslint/prettier chain
scoped to the four new visual-regression files; only this file needed
a change, and eslint + prettier --check are now clean on all of them.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…mports

parseAntdImports' named-import regex required `{` immediately after
import/type, so `import Button, { Divider } from 'antd';` contributed
nothing to the sweep ledger — silently undercounting any file that
mixes a default and named antd import on one line. Allow an optional
default binding before the braces.

Verified the change is a no-op on the current LEDGER.md: running
ledger.mjs against openmetadata-ui/src/main/resources/ui before and
after the regex change produces byte-identical output (0 files in
this repo currently use the combined default+named import shape).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ting import

When every specifier moves out of an antd import AND a target import
already exists, move-named-imports.js called `j(path).remove()` on the
antd import node, discarding its comments outright. If that import was
the file's first statement, its license header vanished from the
output.

Re-attach any leading comments to the node that becomes the program's
new first statement after removal (prepending them ahead of whatever
comments that node already carries).

Added a regression test with a license header above a first-statement
antd import merging into an already-imported target; it fails against
the old code (header dropped) and passes with the fix.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 28, 2026 09:53
Comment thread .github/workflows/playwright-visual.yml Outdated
Comment on lines +35 to +36
permissions:
contents: read

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the Permission Block within the jobs as Best practises :)

akash-jain-10
akash-jain-10 previously approved these changes Jul 29, 2026
Job-scoped permissions per DevOps review convention (matches the e2e
workflow's shape). No functional change for this single-job workflow on
plain pull_request - the token stays contents:read either way.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 29, 2026 06:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@sonarqubecloud

Copy link
Copy Markdown

@gitar-bot

gitar-bot Bot commented Jul 30, 2026

Copy link
Copy Markdown
Code Review ⚠️ Changes requested 2 resolved / 3 findings

Establishes Wave 0 foundations for the AntD to ui-core-components migration, adding codemod tooling, a migration ledger, and a Playwright visual regression test suite. Address the js-yaml major version upgrade finding before merging.

⚠️ Quality: js-yaml bumped to brand-new major v5 and forced via resolutions

📄 openmetadata-ui/src/main/resources/ui/package.json:131 📄 openmetadata-ui/src/main/resources/ui/package.json:314

This commit upgrades js-yaml from ^4.2.0 to ^5.2.2 both as a direct dependency (line 131) and as a global resolutions override (line 314), forcing every transitive consumer onto a major release that was published only days ago. js-yaml is a production dependency (used in ContractYaml, ODCS/ODPS import modals, and DataContractUtils via load/dump), so this changes runtime YAML behavior despite the PR describing itself as "No product code changes — tooling, docs, test harness, and CI only." A v4→v5 major bump can carry breaking parser/serializer changes, and the resolutions entry additionally overrides transitive deps that may still expect the v4 API. Recommend isolating this dependency change from the tooling-only PR, or explicitly verifying v5 compatibility (parse/serialize behavior and transitive consumers) and pinning the exact version before merging.

✅ 2 resolved
Edge Case: Ledger undercounts combined default+named antd imports

📄 tooling/antd-migration/ledger.mjs:20-34
In parseAntdImports, the named-import regex requires { immediately after import/type, so combined forms like import Button, { Divider } from 'antd'; do not match and their named specifiers (Divider) are never counted. Since this ledger drives migration completeness (a row is 'done' at zero), undercounting can hide remaining antd usage and mark a component prematurely complete. Consider matching an optional default binding before the braces, e.g. /import\s+(?:type\s+)?(?:[A-Za-z0-9_$]+\s*,\s*)?\{([^}]+)\}\s+from\s+['"]antd['"]/gs.

Edge Case: Codemod drops license header on full-move into existing import

📄 tooling/antd-codemods/transforms/move-named-imports.js:35-47
In the full-move branch (all specifiers listed), comments are only preserved on the no-existing-target path via decl.comments = path.node.comments; when a target import already exists the code calls j(path).remove(), which drops any leading comments attached to the antd import — including the license header when the antd import is the file's first statement. The test suite only covers header retention for the no-target replaceWith path. Consider re-attaching path.node.comments to the surviving first import (or the target) before removing.

🤖 Prompt for agents
Code Review: Establishes Wave 0 foundations for the AntD to ui-core-components migration, adding codemod tooling, a migration ledger, and a Playwright visual regression test suite. Address the js-yaml major version upgrade finding before merging.

1. ⚠️ Quality: js-yaml bumped to brand-new major v5 and forced via resolutions
   Files: openmetadata-ui/src/main/resources/ui/package.json:131, openmetadata-ui/src/main/resources/ui/package.json:314

   This commit upgrades `js-yaml` from `^4.2.0` to `^5.2.2` both as a direct dependency (line 131) and as a global `resolutions` override (line 314), forcing every transitive consumer onto a major release that was published only days ago. `js-yaml` is a production dependency (used in ContractYaml, ODCS/ODPS import modals, and DataContractUtils via `load`/`dump`), so this changes runtime YAML behavior despite the PR describing itself as "No product code changes — tooling, docs, test harness, and CI only." A v4→v5 major bump can carry breaking parser/serializer changes, and the `resolutions` entry additionally overrides transitive deps that may still expect the v4 API. Recommend isolating this dependency change from the tooling-only PR, or explicitly verifying v5 compatibility (parse/serialize behavior and transitive consumers) and pinning the exact version before merging.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wave 0: Foundations — visual-regression harness, codemods scaffold, sweep ledger

7 participants